-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: report @deprecated() on non-overloaded class constructors
#20104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While you're at this, side question: how difficult would it be to flag transitive calls where a class is passed as Callable[..., T]? Something like
from typing import Callable, TypeVar
from typing_extensions import ParamSPec, deprecated
T = TypeVar("T")
P = ParamSpec("P")
# operator.call
def call(fn: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T: ...
class A:
@deprecated("...")
def __init__(self) -> None: ...
call(A)I'm sure we do that for functions, so maybe constructors can be trivially included as well? (or does this PR already handle this case?)
We don't actually do this for functions. What really happens is that any reference to a # mypy: disable-error-code=empty-body
from typing import Callable, TypeVar
from typing_extensions import ParamSpec, deprecated
T = TypeVar("T")
P = ParamSpec("P")
# operator.call
def call(fn: Callable[P, T], *args: P.args, **kwargs: P.kwargs) -> T: ...
@deprecated("...")
def f(a: int, b: str) -> None: ...
f # E: [deprecated]
call(
f, # E: [deprecated]
1,
""
)It's not the I originally thought of another case while implementing this, which is whether to report calls like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea of marking only __init__ or __new__ as deprecated did not occur to me when I started working on this. I think it is clearly a valuable extension to the current behaviour. Thanks!
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Huh, thanks! I did not realize it's that simple...
If something accepts a
And this makes sense. Maybe at least literal |
|
Sorry, I really didn't mean to suggest doing it right here and right now - IMO this PR is good to go as-is, checking against Callable will better fit as a follow-up improvement unless it's really a one-line addition, but that doesn't seem to be the case:) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@sterliakov Thank you for the suggestion for supporting transitive callables, I'm +0.5 on this not being interpreted as a false positive, and you're also right that it won't be a one-liner so I won't add this to the PR. While prototyping transitive callables I realised that old-style type aliases with deprecated class constructors weren't being reported. I've summarised the changes in the top comment. |
mypy/checkexpr.py
Outdated
| # separately during overload resolution. `callable_node` is `None` for an overload | ||
| # item so deprecation checks are not duplicated. | ||
| callable_info: TypeInfo | None = None | ||
| if isinstance(callable_node.node, TypeInfo): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you don't actually use callable_info later, this could make a great refers_to_class_callable(node) -> bool helper?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is superseded by implementing reporting for type[ClassWithDeprecatedConstructor]() (79d0cb0), see comment chain starting here for discussion.
|
|
||
| @deprecated("use C2 instead") | ||
| class C: | ||
| @deprecated("call `make_c()` instead") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add a similar test (if there isn't one) in incremental mode? .definition is a little magic (it isn't serialized in cache and is restored in fixup.py instead) - that should not be problematic here, but better have it than discover yet another cache trouble a week later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 5302af3
| class A: | ||
| @deprecated("call `self.initialise()` instead") | ||
| def __init__(self) -> None: ... | ||
| def initialise(self) -> None: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also check self.__init__() inside some A (should be flagged) and B (should not be flagged) methods? And maybe also standalone
a = A()
a.__init__()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Committed 4be62c6 to address this...well kind of. I did not add checks for <instance>.__init__ or <instance>.__new__ (only <Class>.__init__ and <Class>.__new__):
<instance>.__init__doesn't activate the code path for@deprecated()at all, mypy complains that accessing<instance>.__init__is unsound and doesn't analyse the call expression further.<instance>.__new__has some weird complaint when any decorator is added; it looks like mypy thinks<instance>.__new__is an instance method after decoration.
See mypy Playground.
In any case, this is still simply testing for deprecation warnings upon attribute access (the attribute just happens to be a class constructor method, and we can't Deprecate[] class/instance variables yet). I added tests covering general attribute access of methods accessed internally in 9e49aa1. The cases for accessing them externally already exist here:
mypy/test-data/unit/check-deprecated.test
Lines 584 to 614 in 054f721
| [case testDeprecatedMethod] | |
| # flags: --enable-error-code=deprecated | |
| from typing_extensions import deprecated | |
| class C: | |
| @deprecated("use g instead") | |
| def f(self) -> None: ... | |
| def g(self) -> None: ... | |
| @staticmethod | |
| @deprecated("use g instead") | |
| def h() -> None: ... | |
| @deprecated("use g instead") | |
| @staticmethod | |
| def k() -> None: ... | |
| C.f # E: function __main__.C.f is deprecated: use g instead | |
| C().f # E: function __main__.C.f is deprecated: use g instead | |
| C().f() # E: function __main__.C.f is deprecated: use g instead | |
| C().f(1) # E: function __main__.C.f is deprecated: use g instead \ | |
| # E: Too many arguments for "f" of "C" | |
| f = C().f # E: function __main__.C.f is deprecated: use g instead | |
| f() | |
| t = (C.f, C.f, C.g) # E: function __main__.C.f is deprecated: use g instead | |
| C().g() | |
| C().h() # E: function __main__.C.h is deprecated: use g instead | |
| C().k() # E: function __main__.C.k is deprecated: use g instead |
|
Also, what happens if you do the following? @deprecated("xxx")
class A:
@deprecated("yyy")
def __init__(self) -> None: ...
A()I don't see any reason to actually have them both deprecated, just wonder what the output will be - both deprecation warnings stacked on the same line? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seem to be some more cases that need consideration. You could extend your logic further, of course, but maybe you could perform the deprecation check at a more convenient place. However, you mentioned above that you already took different places into account, and during a quick look at the code, nothing obvious came to my mind, too, so I am not sure this is possible.
This comment has been minimized.
This comment has been minimized.
Yes, both are stacked on the same line. The code paths for these two are entirely separate (one activates on the reference expression, the other one activates because the callee expression is determined to have a deprecated constructor). I didn't add a test for this, as I don't think we should be condoning/promising anything about this case. IMO no tests should fail if someone else decides later that the class deprecation message supersedes any deprecation message on attribute/method access. |
| item_result, inferred_arg = self.check_call(callee_type, [arg], [ARG_POS], ctx) | ||
| item_result, inferred_arg = self.check_call( | ||
| callee_type, [arg], [ARG_POS], ctx, is_overload_item=callee_is_overload_item | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callee_is_overload_item in this method doesn't actually activate any code paths. Its addition is for logical consistency and future-proofing any more cases handled here:
Lines 1403 to 1415 in 054f721
| def is_generic_decorator_overload_call( | |
| self, callee_type: CallableType, args: list[Expression] | |
| ) -> Overloaded | None: | |
| """Check if this looks like an application of a generic function to overload argument.""" | |
| assert callee_type.variables | |
| if len(callee_type.arg_types) != 1 or len(args) != 1: | |
| # TODO: can we handle more general cases? | |
| return None | |
| if not isinstance(get_proper_type(callee_type.arg_types[0]), CallableType): | |
| return None | |
| if not isinstance(get_proper_type(callee_type.ret_type), CallableType): | |
| return None | |
| with self.chk.local_type_map: |
The test for future-proofing is this one:
mypy/test-data/unit/check-deprecated.test
Line 834 in 79d0cb0
| [case testDeprecatedOverloadedClassConstructorDecoratingOverloadedFunction] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cannot come up with a case where callee_is_overload_item would actually be required. Could you please give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be required if mypy supported the following:
from typing import Callable, Generic, TypeVar
from typing_extensions import deprecated, overload
F = TypeVar("F", bound=Callable[..., object])
class OverloadWrapper(Generic[F]):
__call__: F
@overload
@deprecated("decorated function must take arguments")
def __init__(self: OverloadWrapper[Callable[[], None]], f: Callable[[], None]) -> None: ...
@overload
def __init__(self, f: F) -> None: ...
@overload
def f(a: str) -> None: ...
@overload
def f() -> None: ...# Currently: Revealed type is "__main__.OverloadWrapper[def ()]" with a deprecation warning
# More ideally: Revealed type is "Overload(def (a: builtins.str), def ())" with a deprecation warning
# Without `callee_is_overload_item`, the ideal case would reveal "Overload(def (a: builtins.str), def ())" but the deprecation warning wouldn't be emitted.
reveal_type(OverloadWrapper(f)))That is, wrapping an overload with a class (rather than a function) while preserving or transforming its signature.
The change was made in anticipation that the ideal case should be supported some day, and wouldn't let a potential deprecation report slip by. However, it may be a bit premature on my part?
This comment has been minimized.
This comment has been minimized.
| from typing import Callable, Union | ||
| from typing_extensions import TypeAlias, deprecated | ||
|
|
||
| def maybe() -> bool: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you just use this function to make optional types. Wouldn't it be enough to just use Optional[X] or Union[X, None]?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want mypy to see A as a CallableType for the union in the following example:
def maybe() -> bool: ...
class A: ...
reveal_type(A) # "def () -> __main__.A"
maybe_A = A if maybe() else None
reveal_type(maybe_A) # "def () -> __main__.A | None"As I understand it, there is no way to annotate this. I don't want type[A] | None, that risks leaving some code paths unactivated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, you can use if int() instead of a custom bool-returning function, that "hack" is used in almost every test file and even documented... somewhere.
|
|
||
| def maybe() -> bool: ... | ||
|
|
||
| # `builtins.issubclass()` does not type-narrow properly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope it does:
class A: ...
class B(A): ...
x: type[A]
assert issubclass(x, B)
reveal_type(x) # note: Revealed type is "type[temp.B]"There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is for the following example:
def maybe() -> bool: ...
class A: ...
class B: ...
A_or_B = A if maybe() else B
if issubclass(A_or_B, A):
reveal_type(A_or_B) # Revealed type is "def () -> __main__.A | def () -> __main__.B"| [builtins fixtures/tuple.pyi] | ||
|
|
||
|
|
||
| [case testDeprecatedClassConstructorTypeNarrowing] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test address anything specifically related to @deprecated? It appears to me that it just checks expected issubclass-like functionalities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand from certain tests (e.g. in [case testDeprecatedFunction], [case testDeprecatedMethod] and [case testDeprecatedBaseClass]), some of the test cases or their sub-cases in this file check user-facing reporting without too many specifics of the implementation.
Specifically, calls (e.g. here), method access through instances (e.g. here), and base class access (e.g. here) did not seem to directly test deprecation, as the implementation will emit a report as soon as a reference to a deprecated class/function or a type application to a deprecated class appears anywhere.
I added [case testDeprecatedClassConstructorTypeNarrowing] along this idea (the user should be seeing deprecation messages in the correct location when they've type-narrowed a class object, false positives would be quite distracting), but I'm happy to reduce or remove this.
mypy/checkexpr.py
Outdated
| if isinstance(callable_node, RefExpr) and (callable_node.fullname in ENUM_BASES): | ||
| if callable_node.fullname in ENUM_BASES: | ||
| # An Enum() call that failed SemanticAnalyzerPass2.check_enum_call(). | ||
| return callee.ret_type, callee |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please revert this.
| if (not is_overload_item) and callee.is_type_obj(): | ||
| self.chk.warn_deprecated(callee.definition, context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very nice now!
Maybe a little bit more readable: callee.is_type_obj() and not is_overloaded_item.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I didn't initially want to sprinkle an extra method parameter everywhere, but was necessary for supporting type[ClassWithDeprecatedConstructor]().
Maybe a little bit more readable
I wouldn't usually write it this way. Here, the majority of time that check_callable_call is called, is_overload_item is False, and checking booleans is much cheaper than the method call callee.is_type_obj(). This was an easy short-circuit which I thought wouldn't impact readability with the parentheses added around (not is_overload_item).
| item_result, inferred_arg = self.check_call(callee_type, [arg], [ARG_POS], ctx) | ||
| item_result, inferred_arg = self.check_call( | ||
| callee_type, [arg], [ARG_POS], ctx, is_overload_item=callee_is_overload_item | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A cannot come up with a case where callee_is_overload_item would actually be required. Could you please give an example?
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Fixes #20103
Enables reporting
@deprecated()when non-overloaded class constructors (__init__,__new__) are implicitly invoked (Class()).Previously, to get deprecation warnings when constructing a class,
@deprecated()required to be decorated on the class. However, this requirement also means any usage of the class (in annotations or subclassing) also reported deprecations, which is not always desirable.Now,
@deprecated()can be used to report specifically class construction (rather than all usages of a class). This allows reporting to the user that alternative factory functions or methods exist for building instances.Overloaded class constructors already show deprecation reports (fixed in #19588) and is not addressed here.
Last review was for 53622b1 and since then I've updated the PR with the following:
Only use the constructor which mypy chose from
__init__and__new__(8f08c60). This causes the following false negative:This saves 2 MRO walks as well as keeping type-check reporting consistent (mypy chooses the lowest-indexed item in the MRO out of
__new__and__init__for the signature and subsequent error reports, and currently doesn't warn if__new__and__init__don't match (playground)). A more principled approach would be to build a union of the first__new__and__init__found in the MRO as well as the metaclass's__call__, but that applies generally to constructor signature type-checking and beyond the scope of this PR.Allow deprecation reporting when old-style type aliases (so assignments
A = MyClassorA: TypeAlias = MyClass) are used directly as class constructors (b0ffd6b).Allow deprecation reporting for
type[ClassWithDeprecatedConstructor]()(79d0cb0).